why do we need to invalidate basic memh keys? I'd expect that this will break (at least performance) of some (at least atomics) operations on the base key

I think it should not hinder performance, because base memh rkeys will be recreated on next mkey_pack, like is done in my test.
But you are right, strictly speaking it's NOT necessary to transfer rkeys ownership to the derived memh. It will work even without this transfer. I will remove this ownership transfer logic

The reason why I added this transfer is the following:
Currently it's UCT layer that decides whether to create rkeys (atomic + indirect), and conditions for them are different:

• atomic rkey is created if user requested it (with UCT_MD_MEM_ACCESS_REMOTE_ATOMIC), and hardware supports it. So it might be created no matter if invalidation is needed
• indirect rkey is created only when invalidation is needed
  Now from UCP layer we assume that we will create a memory window when invalidation if needed from EP config. By this time atomic rkeys on base memh might be already created and I thought it's a good idea to move it to the memory window, to avoid re-creating this key per MW.

I removed ownership transfer for now, let's consider this later

i do not think it is correct. base memh should not be modified once derived memh is created

ok, fixed this

do we need invalidation on first mkey pack of base?

do we need test with packing without invalidation first?

In terms of UCT invalidation would mean that we destroy derived memh. Currently I don't have tests with invalidation, actually it's a good point to add them.
Or I misunderstood your comment

params is not checked to be non-NULL in other places. I think we can remove this check here as well.

Yes, initially I didn't have this check, added it to fix NULL pointer crash in CI

Do you remember the failed test? Looks like an issue in caller function.

I don't remember it, but I just see my commit named "Fix NPE" that I've made to address that failure: 53a4b08

Ok, found and fixed this NULL pointer error in gtest: b193dea

can't we modify the access flags when creating a memory window? i would expect yes

I was not aware of this use case.
Do you propose to update this documentation? Or extend derived memh API so that we can create it with alternate access rights?

IMO no need to check (params == NULL)

Same comment I got from Raul)
I added this check intentionally, because some unit test was triggering this call with NULL param.
Ok, let me remove it, so we see exactly that test

Ok, found and fixed this NULL pointer error in gtest: b193dea

should we make this a common macro and use in other memory domains that dont support deriver memh?

Yes, we can
I think we can use this new macro only in those MDs which advertise INVALIDATION support, but do not properly handle it. These are namely: cuda_ipc_md and cma_md

can we make better names, instead of uct_ib_memh_alloc_internal+uct_ib_memh_alloc - uct_ib_memh_alloc+uct_ib_memh_new/init

uct_ib_memh_alloc is an existing function, so we don't want to change its name
I just extracted some common part from uct_ib_memh_alloc into _internal so that it can be reused by uct_ib_memh_clone.

Maybe we name it uct_ib_memh_alloc_common?

1. clone seems wrong - if the derived memh is shallow, why need to fully copy the original memh?
2. seems weird that we use calloc() and then override everything with memcpy

Answering in the opposite order:
2. Right, here malloc would be enough but I'm reusing existing function uct_ib_memh_alloc_internal that does calloc and calculates the size, just to reuse the common code. I think the overhead is minimal.
We could split size calculation into a separate function etc, but I think it's not worth the effort

1. Ok, maybe it's a terminology issue here
   My understanding is:
   Deep copy creates an independent instance of an object, that can be used apart from the original.
   Shallow copy creates an "alias" that depends on master copy and cannot be used separately.
   The latter is what's implemented in this PR, whatever we name it.

Derived memh is a shallow copy, because it makes a shallow copy of MRs - the most important part of original memh. And original object remains the only owner of the MRs state.
Of course there could be different implementations of derived memh. For example, we can imagine a shallow copy looking like that:

struct {
   uct_ib_mlx5_devx_mem_t *base;
   // derived specific fields
} uct_derived_memh;

Looks ok, right? Still you need to allocate this object.
What are the issues with this approach:

• There are places where we assume that memh has always the uct_ib_mem_t base: uct_rc_mlx5_txqp_tag_inline_post: ((uct_ib_mem_t*)iov->memh)
  So we must add uct_ib_mem_t super field to our copy
• The copy must handle a set of rkeys independently from the original memh, meaning that the following fields also needs to be copied:

    struct mlx5dv_devx_obj      *atomic_dvmr;
    struct mlx5dv_devx_obj      *indirect_dvmr;
    uint32_t                    atomic_rkey;
    uint32_t                    indirect_rkey;

This is just to show you that we need to duplicate the significant part of the original memh anyway.

• Then in each and every place we need to check whether passed memh is derived or original (because their layout is different), adding a lot of boilerplate code and CPU overhead..
• Then we need to modify quite some existing functions for key generation, because they cache keys in the original object.

To overcome all these issues I make a shallow copy of the original memh (== they have the same memory layout), and use it interchangeably in all the places. This allows me to minimize the amount of ifs in the code, basically we check for derived handle only during init and cleanup. This helps to avoid any refactoring in the existing functions.

I think we nee to brainstorm the approaches. I don't like having structs where only some fields used, maybe need a deeper refactoring in the IB memh structure.

UCT_MD_FLAG_REG_DERIVED

done

( )

ok